Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Upgrade to pex 1.4.8 and eliminate workarounds. #6594

Merged
merged 2 commits into from
Oct 8, 2018
Merged

Upgrade to pex 1.4.8 and eliminate workarounds. #6594

merged 2 commits into from
Oct 8, 2018

Conversation

jsirois
Copy link
Contributor

@jsirois jsirois commented Oct 4, 2018 •
edited
Loading

Pex now allows us to float setuptools high and run on python 3.7 and
it supports sane platform expansion as well as graceful handling of
non-standard setuptools platform reporting for Apple system
interpreters allowing us to eliminate several workarounds.

We also pickup a fix for concurrent pex extraction which is particularly
useful when running pants from a pex using PEX_FORCE_LOCAL.

Fixes #5922
Fixes #6363
Fixes #6397

@jsirois
Copy link
Contributor Author

jsirois commented Oct 5, 2018

Eeenteresting!: nedbat/coveragepy#715

I can hack around this. Fix forthcoming.

Eric-Arellano
Eric-Arellano approved these changes Oct 5, 2018
View reviewed changes
Copy link
Contributor

@Eric-Arellano Eric-Arellano left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great that this closes a couple issues and kills off so much code! Thank you John.

I see you are working on fixing coverage library itself. If you want to merge this before that goes through, no worries on waiting for upstream. Seems like that could be a followup PR.

src/python/pants/backend/python/pex_util.py Show resolved Hide resolved
@jsirois
Copy link
Contributor Author

jsirois commented Oct 5, 2018

I see you are working on fixing coverage library itself. If you want to merge this before that goes through, no worries on waiting for upstream. Seems like that could be a followup PR.

Yeah - I will not block. I think Ned takes a pretty conservative approach, especially when there is a workaround. For example:

pants/src/python/pants/backend/python/tasks/coverage/plugin.py

Lines 34 to 47 in 0390517

# TODO(John Sirois): Kill the workaround overrides below if there is a useable upstream
# resolution to:
# https://bitbucket.org/ned/coveragepy/issues/646/modifying-coverage-reporting-for-python
@property
def parser(self):
if self._parser is None:
self._parser = PythonParser(filename=self.filename)
self._parser.parse_source()
return self._parser
def no_branch_lines(self):
return self.parser.lines_matching(join_regex(DEFAULT_PARTIAL[:]),
join_regex(DEFAULT_PARTIAL_ALWAYS[:]))

stuhood
stuhood approved these changes Oct 5, 2018
View reviewed changes
Copy link
Member

@stuhood stuhood left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome!

@jsirois
Copy link
Contributor Author

jsirois commented Oct 5, 2018 •
edited
Loading

... and bit by an unrelated jupyter failure which I was working on independently anyhow:

                     E   	Exception message: Could not satisfy all requirements for prompt-toolkit<2.0.0,>=1.0.4:
                     E   	    prompt-toolkit<2.0.0,>=1.0.4(from: ipython<6), prompt-toolkit<2.1.0,>=2.0.0(from: jupyter==1.0.0->jupyter-console)

Fix forthcoming for that in #6600; then I'll rebase here to pick up green CI.

@jsirois
Upgrade to pex 1.4.8 and eliminate workarounds.
bfb214e 
Pex now allows us to float setuptools high and run on python 3.7 and
it supports sane platform expansion as well as graceful handling of
non-standard `setuptools` platform reporting for Apple system
interpreters allowing us to eliminate several workarounds.

We also pickup a fix for concurrent pex extraction which is particularly
useful when running pants from a pex using `PEX_FORCE_LOCAL`.

Fixes pantsbuild#5922
Fixes pantsbuild#6363
@jsirois
Hack around longstanding coverage gotcha.
af47584 
We were always having our 1st sys.path entry nuked, but it just so
happened the 0th position used to be held by the pex bootstrap which was
no longer needed by the time coverage ran.
@jsirois jsirois merged commit 38ccf42 into pantsbuild:master Oct 8, 2018
@jsirois jsirois deleted the issues/5922 branch October 8, 2018 13:19
@Eric-Arellano Eric-Arellano mentioned this pull request Mar 16, 2019
Merged
Eric-Arellano added a commit to pantsbuild/setup that referenced this pull request Mar 16, 2019
@Eric-Arellano
No longer pin setuptools as modern Pants supports modern setuptools (#38
05f3f71 
)

### Problem
We originally had to manually pin setuptools to 5.4.1 to workaround pantsbuild/pants#3948.

This is no longer the case thanks to multiple changes to Pex and Pants, most recently seen at pantsbuild/pants#6594.

Continuing to pin to 5.4.1 prevents Python 3 from working, as seen with https://travis-ci.org/pantsbuild/setup/jobs/507117401#L553.

### Solution
Remove all custom code resolving setuptools and let Pip install and resolve as it normally does.

1. 14 fewer lines of custom code, making the script easier for people to understand and for us to develop.
    * Note `pip` ships with `setuptools` by default for a reason. It's a sensible default.
1. We end up with the same version as Pants after all is done, which is the behavior we want.
1. If we ever needed to bump the pinned version, we would need people to curl the script again, which is very infrequent for them to do.

### Result
Because we pin `setuptools` in Pants' `requirements.txt`, we end up getting the exact same setuptools that Pants is using for that version.

This is the output after creating a new virtual env:
```
Successfully installed Markdown-2.1.1 Pygments-2.3.1 ansicolors-1.0.2 asn1crypto-0.24.0 asttokens-1.1.13 certifi-2019.3.9 cffi-1.11.1 chardet-3.0.4 configparser-3.5.0 contextlib2-0.5.5 cryptography-2.6.1 docutils-0.12 enum34-1.1.6 fasteners-0.14.1 faulthandler-2.6 future-0.17.1 idna-2.8 ipaddress-1.0.22 monotonic-1.5 packaging-16.8 pantsbuild.pants-1.15.0.dev4 pathspec-0.5.9 pex-1.5.3 ply-3.11 psutil-5.4.8 py-zipkin-0.17.0 pycparser-2.19 pyopenssl-17.3.0 pyparsing-2.3.1 pystache-0.5.3 pywatchman-1.4.1 requests-2.21.0 scandir-1.2 setproctitle-1.1.10 setuptools-40.4.3 six-1.12.0 subprocess32-3.2.7 thriftpy2-0.4.2 twitter.common.collections-0.3.10 twitter.common.confluence-0.3.10 twitter.common.dirutil-0.3.10 twitter.common.lang-0.3.10 twitter.common.log-0.3.10 twitter.common.options-0.3.10 urllib3-1.24.1 wheel-0.31.1 www-authenticate-0.9.2

New virtual environment successfully created at /Users/eric/.cache/pants/setup/bootstrap-Darwin-x86_64/1.15.0.dev4_py27.
```
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@baroquebobcat baroquebobcat baroquebobcat approved these changes

@stuhood stuhood stuhood approved these changes

@Eric-Arellano Eric-Arellano Eric-Arellano approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
4 participants
@jsirois @baroquebobcat @stuhood @Eric-Arellano